fix(input-handler): bound URL, zip, and git ingest paths#164
Conversation
56cca8f to
cf31832
Compare
|
Force-pushed v2 changes
Two new tests
Other security-review findings — disposition
Verification
Ready for re-review. |
rng1995
left a comment
There was a problem hiding this comment.
Verdict: Approve — excellent, security-aware DoS hardening that fails closed.
What's good
- Streamed chunked download with an up-front
Content-Lengthcheck and a streamed byte counter (src/skillspector/input_handler.py, ~L145-170), so an oversized/mislabeled body aborts before buffering in memory; partial file is cleaned up on breach (~L172-182). - Zip: member-count cap + summed-uncompressed-size cap before
extractall(~L212-224); post-clone directory-size check with cleanup for git (~L96-111);_directory_size_bytesis symlink-safe (~L234-249). IngestLimitExceededError(ValueError)keeps back-compat with existingValueErrorcallers. Clear, documented constants and README note.
Non-blocking (security-relevant)
- The zip check sums
ZipInfo.file_size(the declared central-directory uncompressed size, ~L218). A crafted bomb that under-declares uncompressed size could still inflate onextractall. For full protection, consider per-member streamed extraction with a running on-disk byte counter that aborts mid-extract. (The forged-central-directory test exercises the declared-size path, not the under-declared case.)
Coordination
Overlaps #159 (this PR lacks the SSRF host allowlist #159 adds) and #163. Reconcile on merge so the final _download_file/_extract_zip has both SSRF allowlisting and ingest size bounds.
Tests
Thorough: MockTransport download caps (header vs streamed), partial-file cleanup, forged central-directory zip bomb, member-count bomb, oversize-clone cleanup. LGTM.
|
Please resolve the merge conflicts |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Summary
This PR bounds the three ingest paths in InputHandler (URL download, zip extraction, git clone) with INGEST_MAX_BYTES (100 MiB) and INGEST_MAX_ZIP_MEMBERS (10,000), raising a fail-closed IngestLimitExceededError (a ValueError subclass). The core design is good: streamed chunked downloads with an up-front Content-Length check plus an authoritative streamed byte counter, partial-file cleanup on breach, zip declared-uncompressed-size and member-count pre-checks before extractall, and a post-clone disk-size check with cleanup. The 8 new tests are thorough and include the partial-file-cleanup and forged-Content-Length cases.
However, the branch is based on a main that predates the SSRF/zip-slip hardening that has since merged, and it cannot be merged or safely evaluated as-is. (Note: this supersedes the earlier automated approval on this PR, which predated the conflicting security work on main.)
Blockers
- Merge conflict (mergeable_state: dirty). The maintainer already asked for conflicts to be resolved on 2026-06-23 and the branch has not been updated since. Main now contains
c4aab55(URL host allowlist + SSRF validation + zip-slip member check),f5305b9(follow_redirects=Falseto close an SSRF redirect bypass), and680cc0f(scp-style git URL host validation), all touching the same functions this PR rewrites. - Conflict resolution is security-sensitive. The PR's
_download_filestill constructshttpx.Client(follow_redirects=True, ...)and has no_validate_url_host()calls, and its_extract_ziplacks main's zip-slip member-path loop. A rebase that takes this PR's side of those hunks would silently reintroduce the SSRF redirect bypass, drop the download/git host allowlist, and drop zip-slip protection. The rebase must preservefollow_redirects=False, both_validate_url_host()call sites, and the zip-slip loop alongside the new size checks. - New tests fail against current main. Every download-path test resolves
https://example.com/..., but main's_download_filerejects any host not inALLOWED_DOWNLOAD_HOSTS(and_is_private_ipdoes a real DNS lookup) before the mocked transport is ever reached. After rebase these tests need an allowlisted host (e.g.raw.githubusercontent.com) and should monkeypatch the host/IP validation to avoid network-dependent DNS in unit tests. The claimed "728 passed" run was against the stale base.
Non-blocking
_directory_size_bytesdocstring says "Path.is_file()returnsTrueonly for regular files" — incorrect;is_file()follows symlinks. The code is only correct because of the explicitnot p.is_symlink()guard; fix the docstring so a future refactor doesn't remove the guard based on the stated rationale.- The clone size check is post-hoc: the full tree lands on disk before rejection, so within the 60s timeout an attacker can still consume disk transiently. Worth a code comment documenting this residual window (the PR body acknowledges it; the code should too).
- The size walk counts
.git/objects toward the cap, so a legitimate repo with a working tree just under 100 MiB can still be rejected; consider excluding.gitor documenting the behavior. - Zip declared-size bound is sound for stdlib extraction (CPython's
zipfiletruncates output at the declaredfile_size), so the forged-central-directory test is meaningful — nice.
Please rebase onto current main, reconcile with the merged SSRF/zip-slip work, fix the tests to pass against the allowlist, and re-run the full suite on the rebased branch.
| download_path = temp_dir / "_download.partial" | ||
| content_type = "" | ||
| try: | ||
| with httpx.Client(follow_redirects=True, timeout=30) as client: |
There was a problem hiding this comment.
This branch predates main's SSRF hardening: main now uses follow_redirects=False (commit f5305b9, closing a redirect-based allowlist bypass) and calls _validate_url_host(url, ALLOWED_DOWNLOAD_HOSTS) at the top of _download_file (c4aab55). When resolving the merge conflicts, both must be preserved — keeping follow_redirects=True here would reintroduce the SSRF bypass.
There was a problem hiding this comment.
Fixed in the rebase (e5e5a21). Both preserved:
httpx.Client(follow_redirects=False, timeout=30)— see_download_filein the rebasedinput_handler.py(the streamed-download wrapper is the only change to that constructor).self._validate_url_host(url, ALLOWED_DOWNLOAD_HOSTS)is the first line of_download_fileafter the docstring, so the allowlist + private-IP check runs before any bytes are read.
The same pattern applies on _clone_git: self._validate_url_host(url, ALLOWED_GIT_HOSTS) runs before subprocess.run.
| extract_dir.mkdir(exist_ok=True) | ||
| try: | ||
| with zipfile.ZipFile(zip_path, "r") as zf: | ||
| infos = zf.infolist() |
There was a problem hiding this comment.
Main's _extract_zip now also iterates zf.namelist() and rejects members that resolve outside extract_dir (zip-slip, c4aab55). The rebased version needs both that loop and these size/member-count checks; taking this PR's side of the conflict would drop zip-slip protection.
There was a problem hiding this comment.
Fixed in the rebase. _extract_zip now runs, in order:
- The
INGEST_MAX_ZIP_MEMBERScheck. - The
sum(info.file_size)vsINGEST_MAX_BYTEScheck. - A
zf.namelist()loop that resolves each member againstextract_dir.resolve()and raisesValueError("...zip-slip...")if the resolved path is not inside the extraction root. zf.extractall(extract_dir).
So the size caps fire first (cheap, protects against bombs before we even touch member names), then zip-slip runs, then extract. Nothing was dropped from main's protections.
| def _directory_size_bytes(path: Path) -> int: | ||
| """Return the total size of all regular files under *path*, in bytes. | ||
|
|
||
| Symlinks are not followed (``Path.is_file()`` returns ``True`` only |
There was a problem hiding this comment.
Docstring is incorrect: Path.is_file() follows symlinks and returns True for a symlink to a regular file. The code below is only safe because of the explicit not p.is_symlink() guard — please fix the rationale here so the guard isn't removed later on the strength of this comment.
There was a problem hiding this comment.
Fixed. _directory_size_bytes docstring rewritten to state exactly what you flagged:
Symlinks are explicitly skipped via
Path.is_symlink()— note thatPath.is_file()follows symlinks and would otherwise returnTruefor a symlink pointing at a regular file, so thenot p.is_symlink()guard is load-bearing and must not be removed. This is what prevents a malicious symlink to/dev/zero(or any large file outside the walked tree) from inflating the count.
So the rationale in the comment now argues for the guard rather than against it — a future refactor that reads this docstring and considers dropping the check will be pushed back toward keeping it.
| # Post-clone size check: a successful --depth 1 clone may still | ||
| # land an arbitrarily large tree on disk. Reject (and clean up) | ||
| # if it exceeds the ingest budget. | ||
| total = _directory_size_bytes(clone_dir) |
There was a problem hiding this comment.
Note this check is post-hoc: the entire clone already landed on disk before it runs, so disk is transiently consumable up to whatever fits in the 60s timeout. Acceptable as a bound, but worth a comment documenting the residual window. Also, .git/ objects count toward the cap, so a legitimate repo with a working tree just under 100 MiB may be rejected.
There was a problem hiding this comment.
Documented as requested. Added an in-code comment right above the size check in _clone_git:
# Post-clone size check: a successful --depth 1 clone may still
# land an arbitrarily large tree on disk before we can measure
# it, so this is a fail-closed cap rather than a hard prefilter.
# Residual window: within the 60s clone timeout an attacker can
# transiently consume up to whatever the network + disk let
# through before this check runs; bounded by the timeout, but
# not zero. `.git/` objects are counted toward the cap, so a
# legitimate repo with a working tree just under `INGEST_MAX_BYTES`
# can still be rejected once packfiles are added.Deliberately not fixing either issue in this PR:
- The residual disk window would require
git clone --filter=blob:limit=…(partial clone) plus streaming the pack, which is a much larger change and mainly buys tighter bounding on the same disk that the 60s timeout already caps. Documented, not addressed. .git/counting toward the cap is arguably correct — an attacker who ships a bloated packfile with a small working tree would otherwise slip through. Excluding.git/would need a separate size accounting pass. Also documented for future callers who hit it on legitimate repos.
Happy to spin either out as follow-up issues if you'd rather have them tracked.
|
|
||
| h = InputHandler() | ||
| try: | ||
| resolved, source_type = h.resolve("https://example.com/skill.md") |
There was a problem hiding this comment.
example.com is not in ALLOWED_DOWNLOAD_HOSTS on current main, so after rebase _validate_url_host raises ValueError before the MockTransport is reached — this and the other download tests will fail. Use an allowlisted host (e.g. raw.githubusercontent.com) and monkeypatch _is_private_ip (it performs a real DNS lookup) so the unit tests stay hermetic.
There was a problem hiding this comment.
Both fixed:
- Host swap. All download-path tests now use
https://raw.githubusercontent.com/...(inALLOWED_DOWNLOAD_HOSTS) instead ofexample.com.sed-style change — six tests updated:test_under_cap_downloads_succeed,test_content_length_header_rejected_before_body_read,test_streamed_body_overflow_rejected_when_header_missing,test_streamed_overflow_leaves_no_partial_file_on_disk,test_download_streams_to_disk_not_memory. - DNS-free unit tests.
_patch_httpx_clientnow also monkeypatchesih._is_private_ipto alambda host: False, so_validate_url_hostdoesn't callsocket.getaddrinfoin the unit-test path. Added a matching_stub_private_ip_check(monkeypatch)helper for the twoTestGitCloneBoundtests.
Also had to touch two pre-existing tests in tests/unit/test_input_handler_ssrf.py (test_raw_githubusercontent_allowed, test_download_does_not_follow_redirects) because _download_file now uses client.stream("GET", url) as a context manager instead of client.get(url) — the mocks needed the nested __enter__ + iter_bytes shape. Same behavioral coverage, updated fixtures.
Full suite result on the rebased branch: 1320 passed, 14 skipped, 6 xfailed. ruff check + ruff format --check clean.
Closes NVIDIA#21. Closes NVIDIA#131. The per-file analysis cap (MAX_FILE_BYTES, 1 MB) sits downstream of InputHandler.resolve(), which pulls scan targets from URLs, zips, or git clones with no size budget of its own. As a result, the per-file cap can be defeated upstream: a multi-GB URL is a memory DoS, a zip bomb fills the disk before extraction is gated, and a large clone lands on disk regardless of the per-file analysis budget. PR NVIDIA#19 explicitly deferred this; this PR addresses the deferred work. Adds two ingest budgets enforced at every remote/archive ingest path: - INGEST_MAX_BYTES (100 MiB): caps streamed URL downloads, total uncompressed size of zip archives, and post-clone disk usage of Git repos. - INGEST_MAX_ZIP_MEMBERS (10,000): caps the number of entries in a single zip (defends against the "many tiny files" zip-bomb variant). Per ingest path: - _download_file streams the body in 64 KiB chunks directly to a temp file inside the existing session temp dir, with a running byte counter. The cap check fires before each chunk is written, so the response body is never accumulated in memory. Content-Length is checked up-front when the server provides it (aborts before reading any body bytes); the streamed counter is authoritative if the header is missing or malformed. A breach mid-stream removes the partial file before the exception propagates, so an attacker who ships exactly INGEST_MAX_BYTES + 1 bytes cannot fill the temp dir. - _extract_zip sums ZipInfo.file_size across all members and checks the member count *before* calling extractall, so classic zip bombs (small archive, huge declared uncompressed size) are rejected without materialising any of the bomb on disk. - _clone_git measures the cloned tree's on-disk size after the existing 60s timeout completes and rejects + cleans up if it exceeds the cap. Symlinks are skipped to avoid runaway counts on malicious /dev/zero-style links. All limit breaches raise IngestLimitExceededError (subclass of ValueError so existing callers that catch ValueError keep working) with a clear message including the limit and the observed size. Adds 10 unit tests covering: under-cap success paths for URL/zip/clone; oversized Content-Length rejected before body read; chunked overflow caught by the streamed counter; *the partial download file is removed when a breach fires mid-stream*; *streaming to disk produces a file of the expected size with no intermediate in-memory concatenation*; declared-uncompressed-oversize zip rejected without extraction; member-count zip-bomb rejected; oversize clone rejected and cleaned up. Full suite: 730 passed, 12 skipped. README documents both caps and their relationship to the per-file analysis cap. Signed-off-by: Rohan Isawe <[email protected]>
cf31832 to
e5e5a21
Compare
|
Pushed rebase
Verification on the rebased branch:
Ready for re-review. |
Summary
Closes #21. Closes #131.
The per-file analysis cap (
MAX_FILE_BYTES, 1 MB) sits downstream ofInputHandler.resolve(), which pulls scan targets from URLs, zips, or git clones with no size budget of its own. The per-file cap can therefore be defeated upstream:client.get(url).contentbuffers the whole body),extractallwith no size pre-check),@wernerkasselman-au flagged this in the review of PR #19 and explicitly deferred it ("the whole ingest layer in
input_handler.pyruns beforebuild_contextever sees a file, and it is unbounded"). PR #19 stayed scoped to the per-file work; this PR addresses the deferred ingest-layer hardening.Changes
New constants
INGEST_MAX_BYTESINGEST_MAX_ZIP_MEMBERSSized above the existing 1 MB per-file analysis cap so legitimate multi-file skills aren't blocked at ingest, but tight enough to bound a memory / disk DoS.
_download_file— streamed with hard capclient.get(url).content(full-body buffering) withclient.stream("GET", url)+ a chunked iterator.INGEST_MAX_BYTES, abort before reading any body bytes.iter_bytes()aborts the read as soon as it crosses the cap._extract_zip— zip-bomb defendedZipInfo.file_sizeacross all members and checks the member count before callingextractall. Classic zip bombs (small archive, huge declared uncompressed size) are rejected without materialising any of the bomb on disk...in their names, this one catches the size/count attack class._clone_git— post-clone size checkshutil.rmtree) if total size exceedsINGEST_MAX_BYTES.p.is_symlink()check) to avoid runaway counts on malicious/dev/zero-style links.Error semantics
All limit breaches raise
IngestLimitExceededError— a subclass ofValueErrorso existing callers that catchValueErrorfromInputHandler.resolve()continue to work. Error messages include both the limit name and the observed size for clarity.Test plan
tests/unit/test_input_handler_bounds.py:httpx.MockTransportwith a forged header).file_sizefield).ruff checkclean.ruff format --checkclean.README
Added a short "Size limits" subsection under Basic Usage documenting both caps and their relationship to the per-file analysis cap (per the maintainer's deferred-work request: "Document in the README that 50 MiB is a per-file analysis limit, not an ingest limit").
Notes / follow-up
--ingest-max-bytes/--ingest-max-zip-membersflags or env vars (SKILLSPECTOR_INGEST_MAX_BYTES), happy to add — wanted to keep this PR scoped.